-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add tuple
argument to ip_network
#8634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit uneasy about this change. It's true that it works at runtime, but passing a tuple
in here seems to be entirely undocumented and untested, so I feel like it could change at any time without any backwards-compatibility guarantees.
I don't have any opinion really |
We should in general type things as the implementation does it, not as documented. I think it applies here too. I also don't see any reason why tests for this function should check with all types that If someone still feels "uneasy" about this, we could add a test case -- again, nothing too thorough, could be for example the two examples in the issue. |
And I just realized that adding a test case to typeshed would be pointless, as those don't run at runtime :) I still think we should support this, because it works. |
I'm fine with this being merged, just don't necessarily want to be the one to hit "merge" myself :) I agree that in general, we should follow the implementation rather than the documentation. |
Passing in a tuple seems to be a common pattern: https://grep.app/search?q=ip_network%28%28 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm convinced.
🥳 |
The discussion here led me to believe that a tuple i.e., the two-tuple support won't go away without breaking backward compatibility. |
Source:
Closes #8570